Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clang transpiler integration #756

Open
wants to merge 26 commits into
base: development
Choose a base branch
from

Conversation

vyast-softserveinc
Copy link

Description

This pull request is aimed for integration occa-transpiler library for providing full C++ support under the OCCA

Added:

option for switching between old & new transpiler transpiler-version

@deukhyun-cha
Copy link
Contributor

cmake -DOCCA_CLANG_BASED_TRANSPILER=ON worked for me to get the new transpiler source and generate the build.

README.md Outdated Show resolved Hide resolved
cmake/GitSubmodules.cmake Outdated Show resolved Hide resolved
examples/cpp/31_oklt_v3_moving_avg/main.cpp Outdated Show resolved Hide resolved
src/occa/internal/bin/occa.cpp Show resolved Hide resolved
Copy link
Contributor

@deukhyun-cha deukhyun-cha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kris-rowe, with this initial pass I think it's ready for you and others to take a look. I have tested a successful compilation of OCCA with this cmake option turned on, and without the option enabled it should have no effect. Please let us know if you can think of any other tests or changes to have, otherwise having this in would help us proceed with our new okl kernel development. Thanks!

@amikstcyr
Copy link
Contributor

Hi - Any hope that this gets merged?

@kris-rowe
Copy link
Member

Please take a look at this issue.

@YuraCobain
Copy link

@kris-rowe the issue is addressed, please take a look and try the fix.

@amikstcyr
Copy link
Contributor

amikstcyr commented Aug 27, 2024

@kris-rowe all issues were addressed, can we please have a conclusion on this?

@IuriiKobein
Copy link

Hi @kris-rowe
If you have any additional comments, questions or concerns I am glad to resolve to merge the PR.

@thilinarmtb
Copy link
Collaborator

Hi @IuriiKobein, I am planning to test this branch soon. I will let you know if I run into any issues.

@thilinarmtb
Copy link
Collaborator

With your latest fix, the tests pass for HIP, Serial and OpenMP backends. I will test this a bit more.

@thilinarmtb
Copy link
Collaborator

@IuriiKobein : I added a simple kernel which calculates the dot product between two
vectors here. Seems like it fails with the transpiler. The failure is due to transpiler not
recognizing unsigned int. I think OCCA supports unsigned int (I may be wrong).

@YuraCobain
Copy link

@thilinarmtb please refer the issue reported above for clarification.

@thilinarmtb
Copy link
Collaborator

Is transpiler version 2 is the same as regular OCCA? Seems like unsigned

@thilinarmtb please refer the issue reported above for clarification.

We will continue the discussion there till the issue is resolved.

@thilinarmtb
Copy link
Collaborator

thilinarmtb commented Sep 30, 2024

I am fine with merging this. I can add a few more tests after the PR gets merged. Below are a
few minor comments I have.

Probably we should figure out the minimum versions of CMake and gcc required and add those
versions to CMakeListsts.txt and documentation. For example, I don't think we need a CMake
version as new as the one currently used in this PR.

Also, it is worth mentioning that you have to build clang and the occa-transpiler using the same
compiler. I don't know if this is an actual requirement. But I had to do so in order to run it on my
testing machine.

@IuriiKobein
Copy link

Hi @thilinarmtb
I have lowered cmake version to be same as OCCA has.
Also added notes regarding minimum GCC version and to use the same version of compiler to build clang and transpiler itself.

@thilinarmtb
Copy link
Collaborator

@IuriiKobein : Thank you very much for the changes. I will merge the PR once the tests pass.

@IuriiKobein
Copy link

Hi @thilinarmtb @kris-rowe
Do you have any updates regarding this PR?
Thanks

@thilinarmtb
Copy link
Collaborator

thilinarmtb commented Oct 15, 2024

I realized that the occa-transpiler is not tested in GitHub CI. I am trying to
test it here. I am running into a bunch of errors (which I think is due to some
header file conflict). I don't run into this issue locally.

@IuriiKobein
Copy link

IuriiKobein commented Oct 15, 2024

@thilinarmtb could you please test default compiler flags from OCCA cmake on CI?

-- C flags : -Wall -Wextra -Wunused-function -Wunused-variable -Wwrite-strings -Wfloat-equal -Wcast-align -Wlogical-op -Wshadow -Wno-c++11-long-long -O3 -DNDEBUG
-- CXX flags : -Wall -Wextra -Wunused-function -Wunused-variable -Wwrite-strings -Wfloat-equal -Wcast-align -Wlogical-op -Wshadow -Wno-unused-parameter -fno-strict-aliasing -O3 -DNDEBUG

@thilinarmtb
Copy link
Collaborator

@IuriiKobein : It worked. But it takes about 18 minutes to build OCCA with transpiler
enabled. One option is to package transpiler as a conda package, install it on GitHub
CI runners (this will be fast) and then build OCCA by linking with the transpiler library.
I can package the transpiler to a conda package if you are interested in taking this route.

@IuriiKobein
Copy link

I appreciate it if you could handle this approach.
We plan to speed up a compilation time of transpiler after initial merge to OCCA.

BTW on average machine with 16 logical i7 cores it takes about 3 minutes so it is a little bit of suprise why on CI it is in times slower.

@thilinarmtb
Copy link
Collaborator

My build using 16 parallel processes failed in GitHub CI. I tried both 8 and 4 processes
and it worked but took about 18 minutes. See here.

I will open a few minor issues on occa-transpiler repo in order to fix a few things
before going ahead with a conda package.

@IuriiKobein
Copy link

Issues are fixed and PR is updated.

@thilinarmtb
Copy link
Collaborator

Thanks @IuriiKobein. I will go ahead with the conda package.

@IuriiKobein
Copy link

Hi @thilinarmtb
I have addressed all reported issues.
Any chance to merge the PR?

@thilinarmtb
Copy link
Collaborator

thilinarmtb commented Oct 24, 2024

@IuriiKobein : I created this PR against your branch to link occa-transpiler
as a third party library. Please let me know if you have any comments. Otherwise,
please merge it.

@IuriiKobein
Copy link

Thanks @thilinarmtb.
I have a small comment to leave link to a repo of new transpiler in 'Building with Clang transplier option' section.
With this change I am happy to merge the PR.

@thilinarmtb
Copy link
Collaborator

Thanks @thilinarmtb. I have a small comment to leave link to a repo of new transpiler in 'Building with Clang transplier option' section. With this change I am happy to merge the PR.

I pushed a commit adding the link. Also, should we refer to it as clang transpiler or occa-transpiler? Seems like
we use both. We can fix this later.

@amikstcyr
Copy link
Contributor

Folks, anything missing to get that PR in before year end?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants